-
Notifications
You must be signed in to change notification settings - Fork 153
Add PVC source support for MCPRegistry #2719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2719 +/- ##
==========================================
+ Coverage 56.55% 56.59% +0.03%
==========================================
Files 320 320
Lines 30874 30901 +27
==========================================
+ Hits 17460 17487 +27
Misses 11911 11911
Partials 1503 1503 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dmartinol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
I commented with some concerns about using the configured values, please TAL!
|
Need some adjustment to adapt to the new model:
|
27c80e2 to
632f5b0
Compare
dmartinol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at the REGISTRY.md file to include the new registry type (BTW: if you can, please also replace the "data sources" to "registries", it's another leftover of previous PRs
|
A few notes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
dmartinol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm apart few minor docs to review
5934fdf to
b71ed3d
Compare
|
I fixed the docs and then rebased. I switched back to using registryName for the mount path, which means that if we have two registry files in a single pvc, it has to be mounted multiple times. I had tried using claimName instead of registryName and that allows for each pvc to be mounted just once, but it requires more test refactoring and changes expectations compared with the existing configMap approach. |
dmartinol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
Large PR justification has been provided. Thank you!
Enables MCPRegistry resources to use PersistentVolumeClaims as a data source in addition to existing ConfigMap, Git, and API sources. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Jon Burdo <[email protected]>
Enables MCPRegistry resources to use PersistentVolumeClaims as a data source in addition to existing ConfigMap, Git, and API sources.
🤖 Generated with Claude Code
Large PR Justification